[TF-76] file renames + split PythonConvertible protocol #23891

realdoug · 2019-04-09T16:25:12Z

naming & code organization changes as discussed here

dan-zheng

Thank you! 🙏

dan-zheng · 2019-04-09T16:29:04Z

test/TensorFlowRuntime/python_conversion.swift

I believe you'll need to rename usages of NumpyConversionTests to PythonConversionTests as well.

test/TensorFlowRuntime/python_conversion.swift

dan-zheng · 2019-04-09T16:30:12Z

stdlib/public/TensorFlow/PythonConversion.swift

Suggested change

// This file defines conversion logic between Python types &

// This file defines conversions between Python types and TensorFlow library

// types.

realdoug · 2019-04-09T16:45:12Z

@dan-zheng My bad, noticed the Python/Numpy test naming right after hitting submit. Should be good now.

dan-zheng · 2019-04-09T16:50:20Z

@swift-ci Please test tensorflow

dan-zheng · 2019-04-09T20:58:46Z

@swift-ci Please test tensorflow

rxwei

There's some important issues I missed in the last merged PR.

rxwei · 2019-04-11T07:01:46Z

stdlib/public/TensorFlow/PythonConversion.swift

Suggested change

if(hasLen == true) {

if Python.hasattr(pythonObject, "__len__" == true {

rxwei · 2019-04-11T07:06:48Z

stdlib/public/TensorFlow/PythonConversion.swift

End sentences with a period.

rxwei · 2019-04-11T07:06:59Z

stdlib/public/TensorFlow/PythonConversion.swift

This should be 80 columns long.

@rxwei is there a linter I can install to catch this kind of stuff?

rxwei · 2019-04-11T07:10:36Z

stdlib/public/TensorFlow/PythonConversion.swift

I didn't review the last change closely. Maybe tests broke because TensorShape(0)'s initialization defaulted to TensorShape(0 as PythonObject) somehow?

@rxwei Yep, that is exactly the case. I spent some time looking into how to manipulate the prority order of the initializers, but if you have a suggested approach i can definitely try it out. I settled for just making sure that this initializer successfully passes through to the correct initializer, since TensorShape(0 as PythonObject) should be valid anyway.

It is also possible that the failure was due to this initializer being failable, but the only reliable repro steps are what I outlined here.

I guess the question is what would you want/expect the output of the below program to be?

func doThing(_ arg: PythonObject) { print(type(of: arg)) } func doThing(_ arg: Int32) { print(type(of: arg)) } doThing(1)

Output currently is PythonObject. Perhaps that is the real underlying bug.

I see. There are two bigger problems. First, the TensorFlow module shouldn't logically depend on Python, i.e. there can be targets that require all APIs that use Python be #if'd out. Second, we ultimately want to optimize tensor operations through techniques like graph program extraction, but having this Python layer for the initialization of a simple tensor shape makes it extremely inefficient when we are targeting hardware like TPUs.

I think a better solution would be to split this protocol into two. This has actually been on my mind for over a year now:

PythonConvertible defines conversion to Python; ConvertibleFromPython defines conversion from Python.

TensorShape will then only conform to PythonConvertible, but not the other one, making this integer literal issue go away.

Ah, makes sense. I could definitely work on a PR for the split up of the protocol.

Great. Let's do that!

rxwei · 2019-04-11T07:11:00Z

stdlib/public/TensorFlow/PythonConversion.swift

Matching scalar does seem right to me. Why is this necessary?

@rxwei the reason is that [Int32](1) (and [Int32](1 as PythonObject)) is not valid in Swift, but TensorShape(1) is valid. Or do you mean that you would just prefer [Int32]([pythonObject]) here?

realdoug · 2019-04-12T21:33:26Z

@dan-zheng ok, made the syntax updates here in case you want to merge just this. I'll follow up later this weekend with the protocol split.

realdoug · 2019-04-15T21:27:39Z

@dan-zheng @rxwei this branch has the protocol split on it now.

rxwei · 2019-04-15T21:39:04Z

Oh excellent. We can skip the revert then.

rxwei

This is awesome. Thanks Doug!

stdlib/public/Python/Python.swift

Co-Authored-By: realdoug <dfriedm3@gmail.com>

rxwei · 2019-04-15T21:46:35Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:46:39Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:46:43Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:46:47Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:46:51Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:46:58Z

@swift-ci please test tensorflow Linux

rxwei · 2019-04-15T21:47:05Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:47:42Z

CI seems dead. This PR needs to go in before #24012. @dan-zheng could you help make sure this gets tested?

stdlib/public/Python/Python.swift

Co-Authored-By: realdoug <dfriedm3@gmail.com>

dan-zheng · 2019-04-15T21:55:26Z

@swift-ci Please test tensorflow

dan-zheng · 2019-04-15T21:56:50Z

@swift-ci Please test tensorflow

rxwei · 2019-04-15T21:58:42Z

@swift-ci please test tensorflow

rxwei · 2019-04-15T21:58:50Z

@swift-ci please test tensorflow, seriously

dan-zheng · 2019-04-15T22:10:19Z

@swift-ci Please test tensorflow

EDIT: I think Jenkins is busy. https://ci-external.swift.org/computer/ubuntu-16.04-tensorflow
Once non-PR CI is done, let's try to trigger PR CI.

rxwei · 2019-04-15T22:26:58Z

@swift-ci please test tensorflow

dan-zheng · 2019-04-15T22:42:52Z

@swift-ci Please test tensorflow

dan-zheng · 2019-04-15T22:43:48Z

@swift-ci Please test tensorflow linux

rxwei · 2019-04-16T00:47:31Z

@swift-ci please test tensorflow Linux

dan-zheng · 2019-04-16T06:15:02Z

There is one test failure:

[ RUN      ] PythonConversion.shaped-array-conversion
stdout>>> check failed at /Volumes/BuildData/Users/swiftci/jenkins/workspace/swift-PR-TensorFlow-macOS/swift/test/TensorFlowRuntime/python_conversion.swift, line 62
stdout>>> expected: (2, 7, 2) (of type Python.PythonObject)
stdout>>> actual: [2, 7, 2] (of type Python.PythonObject)
stdout>>> check failed at /Volumes/BuildData/Users/swiftci/jenkins/workspace/swift-PR-TensorFlow-macOS/swift/test/TensorFlowRuntime/python_conversion.swift, line 64
stdout>>> expected: (14, 2) (of type Python.PythonObject)
stdout>>> actual: [14, 2] (of type Python.PythonObject)
[     FAIL ] PythonConversion.shaped-array-conversion
[ RUN      ] PythonConversion.tensor-conversion
[       OK ] PythonConversion.tensor-conversion
[ RUN      ] PythonConversion.tensor-round-trip
[       OK ] PythonConversion.tensor-round-trip
[ RUN      ] PythonConversion.tensor-shape
[       OK ] PythonConversion.tensor-shape
PythonConversion: Some tests failed, aborting
UXPASS: []
FAIL: ["shaped-array-conversion"]
SKIP: []

NumPy arrays have tuple shapes, not list shapes.

dan-zheng · 2019-04-16T07:10:39Z

@swift-ci Please test tensorflow

realdoug · 2019-04-16T13:41:36Z

ugh sorry bout that! Could have sworn that passed locally for me.

) Split `PythonConvertible` into two protocols: * `PythonConvertible`: conversion to Python. * `ConvertibleFromPython`: conversion from Python.

realdoug mentioned this pull request Apr 9, 2019

[TF-76][Python Interop] add PythonConvertible conformance to TensorShape #23762

Merged

dan-zheng approved these changes Apr 9, 2019

View reviewed changes

realdoug force-pushed the tf-76-renames branch from d106509 to dd0e73a Compare April 9, 2019 16:43

dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Apr 9, 2019

dan-zheng approved these changes Apr 9, 2019

View reviewed changes

rxwei requested changes Apr 11, 2019

View reviewed changes

dan-zheng self-requested a review April 12, 2019 05:06

Doug Friedman and others added 5 commits April 15, 2019 19:03

numpy => python

5137b0b

s/Numpy/Python/

cde6b27

other renames that were missed

2e27373

syntax updates

1d49f38

split PythonConvertible protocol in two

7c4f0f8

rxwei mentioned this pull request Apr 15, 2019

Revert "[TF-76][Python Interop] add PythonConvertible conformance to TensorShape" #24044

Closed

realdoug force-pushed the tf-76-renames branch from 2d8e267 to 43f6781 Compare April 15, 2019 21:20

realdoug changed the title ~~[TF-76] numpy => python~~ [TF-76] PythonConvertible conformance for TensorShape Apr 15, 2019

realdoug force-pushed the tf-76-renames branch from 43f6781 to 7c4f0f8 Compare April 15, 2019 21:25

rxwei approved these changes Apr 15, 2019

View reviewed changes

stdlib/public/Python/Python.swift Outdated Show resolved Hide resolved

stdlib/public/Python/Python.swift Outdated Show resolved Hide resolved

stdlib/public/Python/Python.swift Outdated Show resolved Hide resolved

rxwei and others added 3 commits April 15, 2019 17:43

Update stdlib/public/Python/Python.swift

99c2cdb

Co-Authored-By: realdoug <dfriedm3@gmail.com>

Update stdlib/public/Python/Python.swift

aacc10e

Co-Authored-By: realdoug <dfriedm3@gmail.com>

Update stdlib/public/Python/Python.swift

71dc587

Co-Authored-By: realdoug <dfriedm3@gmail.com>

dan-zheng approved these changes Apr 15, 2019

View reviewed changes

stdlib/public/Python/Python.swift Outdated Show resolved Hide resolved

Update stdlib/public/Python/Python.swift

3783c95

Co-Authored-By: realdoug <dfriedm3@gmail.com>

rxwei mentioned this pull request Apr 15, 2019

[TF] Change APIs to use Int instead of Int32. #24012

Merged

realdoug changed the title ~~[TF-76] PythonConvertible conformance for TensorShape~~ [TF-76] file renames + split PythonConvertible protocol Apr 15, 2019

Fix "shaped-array-conversion" test.

27f5521

NumPy arrays have tuple shapes, not list shapes.

dan-zheng merged commit 3c27938 into swiftlang:tensorflow Apr 16, 2019

	// This file defines conversion logic between Python types &
	// This file defines conversions between Python types and TensorFlow library
	// types.

	if(hasLen == true) {
	if Python.hasattr(pythonObject, "__len__" == true {

[TF-76] file renames + split PythonConvertible protocol #23891

[TF-76] file renames + split PythonConvertible protocol #23891

Uh oh!

Conversation

realdoug commented Apr 9, 2019

Uh oh!

dan-zheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realdoug commented Apr 9, 2019

Uh oh!

dan-zheng commented Apr 9, 2019

Uh oh!

dan-zheng commented Apr 9, 2019

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realdoug Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realdoug commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realdoug commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

rxwei commented Apr 15, 2019

Uh oh!

Uh oh!

dan-zheng commented Apr 15, 2019

Uh oh!

dan-zheng commented Apr 15, 2019

realdoug Apr 11, 2019 •

edited

Loading

realdoug commented Apr 12, 2019 •

edited

Loading

dan-zheng commented Apr 15, 2019 •

edited

Loading